Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(client): support local bind for HttpConnector #1500

Merged
merged 1 commit into from
May 1, 2018

Conversation

kw217
Copy link
Contributor

@kw217 kw217 commented Apr 26, 2018

Add set_local_address to the HttpConnector.
This configures the client to bind the socket to a local address of
the host before it connects to the destination. This is useful on
hosts which have multiple network interfaces, to ensure the request is
issued over a specific interface.

Includes an example to show how it is used and regression-check the API (copied from the existing client example). Please let me know if I should be adding any tests, and if so where.

Closes #1498

@kw217 kw217 force-pushed the 1498-add-local-bind branch 2 times, most recently from 95155c4 to d433c2e Compare April 26, 2018 11:00
@kw217
Copy link
Contributor Author

kw217 commented Apr 26, 2018

I'm not sure what caused the build timeout; I don't think it's related to my code.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Don't worry about the test failure, that specific test has been flaky, and I haven't yet been able to track down why it fails when it does...

///
/// Default is `None`.
#[inline]
pub fn set_local_address(&mut self, addr: Option<IpAddr>) {

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's normally called the local address; see bind(2) at https://linux.die.net/man/2/bind and http://pubs.opengroup.org/onlinepubs/009695399/functions/bind.html . Normally bind takes a SockAddr, but for this specific case of binding-before-connecting, it's vanishingly rare that you want to specify the port as well as the IP, which is why I have specified IpAddr. But it's still an address.

@@ -0,0 +1,56 @@
#![deny(warnings)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my personal preference here would be to not include an entire example file, as it means another thing to adjust when changes are required. We don't have an example for every combination of configuration options...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem - it was just there to show what it would look like. I've removed it now.

@kw217 kw217 force-pushed the 1498-add-local-bind branch 2 times, most recently from 82c4bea to a80471a Compare April 30, 2018 12:09
@seanmonstar
Copy link
Member

Woops, test failures are because the Cargo.toml still references the example that you removed XD

Add `set_local_address` to the `HttpConnector`.
This configures the client to bind the socket to a local address of
the host before it connects to the destination. This is useful on
hosts which have multiple network interfaces, to ensure the request is
issued over a specific interface.

Closes hyperium#1498
@kw217 kw217 force-pushed the 1498-add-local-bind branch from a80471a to b6a3c85 Compare May 1, 2018 07:41
@kw217
Copy link
Contributor Author

kw217 commented May 1, 2018

Right, sorry about that. Fixed now - back to the same timeout error you said not to worry about.

@seanmonstar seanmonstar merged commit d3e4089 into hyperium:master May 1, 2018
@seanmonstar
Copy link
Member

Thanks so much!

@kw217 kw217 deleted the 1498-add-local-bind branch May 2, 2018 07:42
@kw217
Copy link
Contributor Author

kw217 commented May 2, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants